fix: Close the original response body on non-2xx responses - #4486
Conversation
CheckResponse substitutes r.Body with a re-readable NopCloser copy on error responses, so asserting on resp.Body.Close alone cannot catch a leak of the network body. Wrap the test client's transport and record whether the body it returned is closed. This test fails at this commit; the fix follows in the next one.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
`CheckResponse` substitutes `r.Body` with a re-readable `NopCloser` copy on every non-2xx response (google#1363). Since google#1772, the error-path `defer resp.Body.Close()` in `bareDo` is registered after that substitution, so the deferred close releases the copy and the network body is never closed. `CopilotService.fetchMetricsReport` and `RepositoriesService.downloadReleaseAssetFromURL` have the same pattern. Consequences of the unclosed network body: - With an `http.Client` that has `Timeout` set and a wrapped transport (which includes clients built via `WithAuthToken`), every non-2xx response parks one `net/http.setRequestCancel.func4` goroutine for the remainder of the timeout. Bounded, but it intermittently fails `goleak`-checked test suites downstream. - Error bodies larger than `maxErrorBodySize` are only partially drained, so the connection is additionally lost. The fix captures the network body before calling CheckResponse and closes that instead. The google#1363 behavior (re-readable error bodies) is unchanged: the substitute is left open for callers.
5c52ef3 to
25b7434
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4486 +/- ##
=======================================
Coverage 98.52% 98.52%
=======================================
Files 195 195
Lines 17729 17732 +3
=======================================
+ Hits 17468 17471 +3
Misses 261 261 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| type closeRecorder struct { | ||
| io.ReadCloser | ||
| closed *bool | ||
| } |
There was a problem hiding this comment.
having this round tripper is also a precondition for the bug. Not a necessary precondition for the test because we're asserting that the underlying cause can't happen - Close() not being called.
gmlewis
left a comment
There was a problem hiding this comment.
Ah, I see what you are saying, @yavorl.
I would like to cover all possible unclosed bodies, if possible, so that this issue can truly be recorded as solved. What are your thoughts as to what it would take to accomplish this?
LGTM.
The fix is the same, I have to see whether the test pattern translates to the other invocations. Would you like me to try it in this PR or a separate follow-up? |
|
Well, I'm fine either way. If you want to keep it separate in a new PR, that is fine with me too. Thank you, @yavorl! |
|
Done it in the same PR. Added a comment as well to the godoc of |
| // CheckResponse substitutes r.Body with a re-readable copy on error | ||
| // responses, so capture the network body first: it is the one that must | ||
| // be closed. | ||
| origBody := resp.Body |
There was a problem hiding this comment.
Could we centralize the fix to one place? I think it's possible to patch only CheckResponse.
There was a problem hiding this comment.
The patch is something like this:
diff --git a/github/copilot.go b/github/copilot.go
index adbfae32..9dd24eb8 100644
--- a/github/copilot.go
+++ b/github/copilot.go
@@ -1212,12 +1212,8 @@ func (s *CopilotService) fetchMetricsReport(ctx context.Context, url string) (*h
return nil, nil, err
}
- // CheckResponse substitutes resp.Body with a re-readable copy on error
- // responses, so capture the original body first: it is the one that must
- // be closed.
- origBody := resp.Body
if err := CheckResponse(resp); err != nil {
- _ = origBody.Close()
+ resp.Body.Close()
return nil, newResponse(resp), err
}
diff --git a/github/copilot_test.go b/github/copilot_test.go
index ee1f804a..4ddbab41 100644
--- a/github/copilot_test.go
+++ b/github/copilot_test.go
@@ -3212,8 +3212,8 @@ func TestCopilotService_DownloadDailyMetrics(t *testing.T) {
}
}
-// CheckResponse substitutes resp.Body with a re-readable copy on error
-// responses; fetchMetricsReport must still close the original body it replaces.
+// CheckResponse closes the original resp.Body and substitutes a re-readable
+// copy on error responses; fetchMetricsReport must not leak the original body.
func TestCopilotService_fetchMetricsReport_closesOriginalBodyOnErrorResponse(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)
diff --git a/github/github.go b/github/github.go
index efb4c677..bfa0bd10 100644
--- a/github/github.go
+++ b/github/github.go
@@ -1282,13 +1282,9 @@ func (c *Client) bareDo(caller *http.Client, req *http.Request) (*Response, erro
c.rateMu.Unlock()
}
- // CheckResponse substitutes r.Body with a re-readable copy on error
- // responses, so capture the network body first: it is the one that must
- // be closed.
- origBody := resp.Body
err = CheckResponse(resp)
if err != nil {
- defer origBody.Close()
+ defer resp.Body.Close()
// Special case for AcceptedErrors. If an AcceptedError
// has been encountered, the response's payload will be
// added to the AcceptedError and returned.
@@ -1795,11 +1791,9 @@ func (e *Error) UnmarshalJSON(data []byte) error {
// API error responses are expected to have response
// body, and a JSON response body that maps to [ErrorResponse].
//
-// On error responses other than 202 Accepted, CheckResponse consumes r.Body
-// and replaces it with an in-memory copy so that the error body can be
-// re-read. Closing r.Body after CheckResponse returns therefore closes only
-// the copy: to release the original body and its underlying connection,
-// capture r.Body before the call and close the captured body instead.
+// On error responses other than 202 Accepted, CheckResponse consumes and
+// closes r.Body, then replaces it with an in-memory copy so that the error
+// body can be re-read by the caller.
//
// The error type will be *[RateLimitError] for rate limit exceeded errors,
// *[AcceptedError] for 202 Accepted status codes,
@@ -1822,9 +1816,11 @@ func CheckResponse(r *http.Response) error {
errorResponse = &ErrorResponse{Response: r}
}
}
- // Re-populate error response body because GitHub error responses are often
- // undocumented and inconsistent.
+ // Close the original body and re-populate the error response body with an
+ // in-memory copy, because GitHub error responses are often undocumented and
+ // inconsistent and the caller may need to re-read them.
// Issue #1136, #540.
+ _ = r.Body.Close()
r.Body = io.NopCloser(bytes.NewBuffer(data))
switch {
case r.StatusCode == http.StatusUnauthorized && strings.HasPrefix(r.Header.Get(headerOTP), "required"):
diff --git a/github/github_test.go b/github/github_test.go
index 776abaf4..77b346c6 100644
--- a/github/github_test.go
+++ b/github/github_test.go
@@ -2308,8 +2308,8 @@ func (r *closeRecorder) Close() error {
return r.ReadCloser.Close()
}
-// CheckResponse substitutes resp.Body with a re-readable copy on error
-// responses; the network body it replaces must still be closed.
+// CheckResponse closes the original resp.Body and substitutes a re-readable
+// copy on error responses; the network body must not leak.
func TestDo_closesOriginalBodyOnErrorResponse(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)
diff --git a/github/repos_releases.go b/github/repos_releases.go
index 3361a5d7..889afb6d 100644
--- a/github/repos_releases.go
+++ b/github/repos_releases.go
@@ -375,12 +375,8 @@ func (s *RepositoriesService) downloadReleaseAssetFromURL(ctx context.Context, f
if err != nil {
return nil, err
}
- // CheckResponse substitutes resp.Body with a re-readable copy on error
- // responses, so capture the original body first: it is the one that must
- // be closed.
- origBody := resp.Body
if err := CheckResponse(resp); err != nil {
- _ = origBody.Close()
+ _ = resp.Body.Close()
return nil, err
}
return resp.Body, nil
diff --git a/github/repos_releases_test.go b/github/repos_releases_test.go
index 4fb85c75..4208513d 100644
--- a/github/repos_releases_test.go
+++ b/github/repos_releases_test.go
@@ -537,9 +537,9 @@ func TestRepositoriesService_DownloadReleaseAsset_FollowRedirectToError(t *testi
}
}
-// CheckResponse substitutes resp.Body with a re-readable copy on error
-// responses; downloadReleaseAssetFromURL must still close the original body it
-// replaces. Unlike its sibling tests, the recorder wraps the follow-redirects
+// CheckResponse closes the original resp.Body and substitutes a re-readable
+// copy on error responses; downloadReleaseAssetFromURL must not leak the
+// original body. Unlike its sibling tests, the recorder wraps the follow-redirects
// client's transport: that client, not the library client, performs the
// redirected request, so wrapping the library client would only ever observe
// the first hop's correctly-closed redirect response and never the leak.
There was a problem hiding this comment.
The reason I didn't do it initially is that this is a behaviour change, callers might start seeing errors where there were none before.
Reason being - Response.Body is only guaranteed to be an io.ReadCloser. Custom transports, test doubles, and direct CheckResponse callers routinely supply non-Transport bodies - so the only close semantics a public API may rely on are io.Closer's, which states that a second Close is undefined behaviour. So if someone was diligent about this before and they closed the body themselves, theoretically they can start seeing errors or panics.
There was a problem hiding this comment.
That said, if you would still prefer to go with that approach after my argument. I'd be happy to apply the patch.
There was a problem hiding this comment.
Fair point. Thanks for the explanation.
There was a problem hiding this comment.
Thank you for the review & thank you for the merge @gmlewis
Fixes #4484
CheckResponsesubstitutesr.Bodywith a re-readableNopClosercopy on everynon-2xx response (#1363). Since #1772, the error-path
defer resp.Body.Close()inbareDois registered after that substitution, so the deferred close releases thecopy and the network body is never closed.
CopilotService.fetchMetricsReportandRepositoriesService.downloadReleaseAssetFromURLhave the same pattern.Consequences of the unclosed network body:
http.Clientthat hasTimeoutset and a wrapped transport (whichincludes clients built via
WithAuthToken), every non-2xx response parks onenet/http.setRequestCancel.func4goroutine for the remainder of the timeout.Bounded, but it intermittently fails
goleak-checked test suites downstream.maxErrorBodySizeare only partially drained, so theconnection is additionally lost. 1Mb seems generous so it can be a non-issue.
The fix captures the network body before calling
CheckResponseand closes thatinstead. The #1363 behavior (re-readable error bodies) is
unchanged: the substitute is left open for callers.
The regression test wraps the test client's transport to observe the network
body and asserts it is closed after an error response. It fails on
masterandpasses with this change; the full package test suite passes.
commit 1: a failing regression test
commit 2: the fix; capture the original body & close it
commit 3: similar regression tests for the other 2 call sites
commit 4: fix the 2 remaining callers & update the
CheckResponse()godoc